Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve console batch mode #1692

Merged
merged 5 commits into from
Dec 19, 2024
Merged

Improve console batch mode #1692

merged 5 commits into from
Dec 19, 2024

Conversation

gramian
Copy link
Collaborator

@gramian gramian commented Aug 17, 2024

What does this PR do?

Overall, this change alters the console behavior such that if an error occurs in batch mode or during a script, execution is halted and if in batch mode, the console is exited.

In particular these changes are introduced in Console.java:

  1. Make parse method have 3 arguments (see line 731). The third argument is a boolean indicating batch mode, which is true by default.
  2. Add wrapper method with 2 arguments to minimize changes (see line 727)
  3. New parse does not consume exeception anymore, but throws them upwards (see line 742ff).
  4. "Lower" execute method now throws also RuntimeExeception (see line line 223)
  5. "Upper" execute method uses new parse method depending on if batch mode is set.

Motivation

Currently console continues to execute commands in scripts or batch even though previous command fails. This can produce unwanted results. Especially tracking errors is complicated since if the last line passes, it seems as if a script or batch is successful.

Additional Notes

@lvca

  • ConsoleTest: testNotNullProperties fails, is this test faulty?
  • Need help constructing tests for this.
  • The "upper" execute method (line 155) should be renamed so there are not two methods names execute.
  • Are the exeception-related changes OK?

Checklist

  • I have run the build using mvn clean package command
  • My unit tests cover both failure and success scenarios

@robfrank robfrank changed the title WIP: Console break on error in batch mode Improve console batch mode Dec 19, 2024
@robfrank
Copy link
Collaborator

I reworked a bit this pr.
now the console has a new flag -fae (fail at end) that is evaulated only when using the batch mode.
If batch mode is enabled:

  • if -fae is added, the whole batch is executed and errors are just printed out
  • otherwise the batch is stopped when an error occurred

@robfrank robfrank marked this pull request as ready for review December 19, 2024 09:44
@gramian
Copy link
Collaborator Author

gramian commented Dec 19, 2024

This looks good; I will test this later today. Did you see the "Additional Notes" above?

Thanks for the improvements!

PS: I will make additions to the docs once this PR is merged.

@robfrank
Copy link
Collaborator

@gramian All the tests are green and I adde a couple of tests in ConsoleBatchTest to verify the new flags. My intention was to not change the previous behavior.

@gramian
Copy link
Collaborator Author

gramian commented Dec 19, 2024

@robfrank If I remember correctly, the testNotNullProperties console test did not actually run, but I will check this again later.

@gramian
Copy link
Collaborator Author

gramian commented Dec 19, 2024

I figured it out. The test runs and seems to work as it is supposed to.

@lvca lvca added the enhancement New feature or request label Dec 19, 2024
@lvca lvca modified the milestones: 24.11.2, 24.12.1, 25.1.1 Dec 19, 2024
@gramian
Copy link
Collaborator Author

gramian commented Dec 19, 2024

I tested with a simple script and it seems to work as expected.

@robfrank robfrank requested a review from lvca December 19, 2024 15:30
@mergify mergify bot merged commit db93d7f into ArcadeData:main Dec 19, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants